src/common/hierarchical/worker_manager.cpp (1)

305-315: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

enqueue_dispatch transiently overshoots capacity.

fetch_add then rollback means a concurrent has_capacity() / idle() observer can see inflight_ == capacity_ + 1 before the rollback lands, so a scheduler gate keyed on has_capacity() may reject a slot that is actually free (and, symmetrically, idle() never reports true spuriously, so the risk is only a missed admission). A CAS loop keeps the counter within bounds.

♻️ Bounded reservation

-    uint32_t previous = inflight_.fetch_add(1, std::memory_order_acq_rel);
-    if (previous >= capacity_) {
-        inflight_.fetch_sub(1, std::memory_order_acq_rel);
-        throw std::logic_error("WorkerThread::dispatch: endpoint capacity exceeded");
-    }
+    uint32_t previous = inflight_.load(std::memory_order_acquire);
+    do {
+        if (previous >= capacity_) {
+            throw std::logic_error("WorkerThread::dispatch: endpoint capacity exceeded");
+        }
+    } while (!inflight_.compare_exchange_weak(
+        previous, previous + 1, std::memory_order_acq_rel, std::memory_order_acquire
+    ));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/common/hierarchical/worker_manager.cpp` around lines 305 - 315, Update
WorkerThread::enqueue_dispatch to reserve inflight_ with a compare-exchange loop
that only increments when the current value is below capacity_. Remove the
fetch_add-and-rollback sequence so concurrent has_capacity() and idle()
observers never see inflight_ exceed capacity_; retain the existing exception,
dispatch ID assignment, queue insertion, and notification behavior.

src/common/hierarchical/worker_manager.h (1)

97-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Declare the protocol version with its wire width.

The frame trailer field is 8 bytes (worker_manager.cpp writes a uint64_t; Python unpacks =Q), so a uint32_t constant forces an implicit widen at every use and invites a 4-byte memcpy if someone passes the constant directly.

♻️ Match the wire type

-static constexpr uint32_t MAILBOX_TASK_PROTOCOL_VERSION = 2;
+static constexpr uint64_t MAILBOX_TASK_PROTOCOL_VERSION = 2;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/common/hierarchical/worker_manager.h` at line 97, Change
MAILBOX_TASK_PROTOCOL_VERSION from uint32_t to uint64_t so its declared type
matches the 8-byte protocol trailer written by the worker-manager serialization
path and read as =Q by Python. Keep its value and existing uses unchanged.

python/simpler/worker.py (4)

1753-1761: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

slot_id < task_frame_count is validated against the parameter, but the frame arrays are sized by _TASK_FRAME_COUNT.

frame_bufs / frame_addrs are built with range(_TASK_FRAME_COUNT) (Line 1740-1742) while identity validation bounds slot_id by task_frame_count. A task_frame_count > _TASK_FRAME_COUNT would admit a slot id that indexes past both lists. Unreachable today (the only caller passes _TASK_FRAME_COUNT), but the two sources of truth should be one.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/simpler/worker.py` around lines 1753 - 1761, The validate_identity
function must bound slot_id using the same _TASK_FRAME_COUNT constant used to
size frame_bufs and frame_addrs, rather than the task_frame_count parameter.
Update that validation while preserving the other identity checks.

---

1888-1892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Silently swallowed abort_prepared failures leave no diagnostic trail.

All four abort paths use bare except Exception: pass. An abort that fails means the backend still holds an unpublished prepared run (device arena/slot not reclaimed), which will surface later as an unexplained slot exhaustion or lease mismatch with no clue about the original cause. Emit the exception to stderr like the other best-effort cleanups in this module do.

♻️ Suggested logging

-                        try:
-                            abort_prepared(prepared_identity)
-                        except Exception:  # noqa: BLE001
-                            pass
+                        try:
+                            abort_prepared(prepared_identity)
+                        except Exception as exc:  # noqa: BLE001
+                            sys.stderr.write(
+                                f"chip_process dev={device_id}: abort_prepared failed: {type(exc).__name__}: {exc}\n"
+                            )
+                            sys.stderr.flush()

Also applies to: 1908-1911, 1949-1953, 1973-1977

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/simpler/worker.py` around lines 1888 - 1892, Update all four abort
cleanup paths around abort_prepared to preserve best-effort exception handling
while emitting each caught exception to stderr, matching the diagnostic pattern
used by other best-effort cleanups in the module. Replace the silent pass blocks
associated with prepared_identity in each path; do not alter the abort flow or
re-raise the failures.

Source: Linters/SAST tools

---

1737-1737: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

prepared_frames is mutated from both the admission and executor threads without synchronization.

The admission thread inserts/pops at Lines 1881/1893/1907 while the executor pops at Lines 1954/1960. Individual dict ops are atomic in CPython, so nothing corrupts today, but the check-then-act pairs (index in prepared_frames → insert, get → compare → pop) are not, and the invariant "one prepared epoch per slot" depends on mailbox-state ordering rather than on any explicit guard. Moving these accesses under action_cv (already held for the queue) makes the ownership rule enforceable rather than incidental.

Also applies to: 1906-1921, 1945-1960

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/simpler/worker.py` at line 1737, Protect all accesses to
prepared_frames in the admission and executor paths with action_cv, including
the check-then-insert/pop sequences and get/compare/pop logic around the
identified admission and executor operations. Ensure each compound operation
executes while holding the condition’s lock, preserving the
one-prepared-epoch-per-slot invariant without changing mailbox ordering.

---

1836-1854: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Admission thread spins with no yield.

admission_loop polls the control word and both frame states in a tight loop with no time.sleep/backoff. In the forked chip child this burns a core per chip and contends for the GIL with the executor thread between its native calls (the executor only drops the GIL inside run_from_blob / execute_prepared). A short bounded sleep once no frame is actionable would keep latency while removing the busy-wait.

♻️ Suggested bounded poll

         def admission_loop() -> None:
             nonlocal control_queued
             while not stop_admission.is_set():
+                progressed = False
                 control_state = _mailbox_load_i32(state_addr)

…and at the end of the frame sweep, if not progressed: time.sleep(_MAILBOX_POLL_INTERVAL_S).

Please confirm what poll cadence the existing _run_mailbox_loop uses so the two loops stay consistent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/simpler/worker.py` around lines 1836 - 1854, Update admission_loop to
avoid tight polling by tracking whether control or frame admission made progress
during each iteration and sleeping for the established mailbox poll interval
when none did. Reuse the cadence used by _run_mailbox_loop via
_MAILBOX_POLL_INTERVAL_S, while preserving immediate handling of actionable
frames and shutdown/control requests.

tests/ut/py/test_callable_identity.py (1)

604-604: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert against the constant rather than the literal 2.

This worker has no device_ids, so _start_hierarchical leaves direct_chip_pipeline_depth at PTO_PIPELINE_MAX_DEPTH. Hard-coding 2 makes the test fail confusingly if that cap ever changes, and hides what the assertion is actually about (no chips ⇒ no depth negotiation, so the cap is passed through).

♻️ Proposed change

-        assert fake_c_worker.pipeline_depth == 2
+        # No device_ids, so no chip negotiation happens and the cap is passed through.
+        assert fake_c_worker.pipeline_depth == worker_mod.PTO_PIPELINE_MAX_DEPTH

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ut/py/test_callable_identity.py` at line 604, Update the assertion for
fake_c_worker.pipeline_depth to compare against PTO_PIPELINE_MAX_DEPTH instead
of the literal 2, preserving the test’s verification that the no-device path
passes through the configured maximum depth.

tests/ut/cpp/hierarchical/test_scheduler.cpp (1)

721-722: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer a bounded copy into the fixed-size output_prefix.

std::strcpy is flagged by static analysis; std::snprintf(diagnostic_config.output_prefix, sizeof(diagnostic_config.output_prefix), "%s", "/tmp/simpler-diagnostic-successor") keeps the intent and removes the unbounded-write pattern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ut/cpp/hierarchical/test_scheduler.cpp` around lines 721 - 722, Replace
the unbounded std::strcpy assignment to diagnostic_config.output_prefix with a
bounded std::snprintf call using sizeof(diagnostic_config.output_prefix),
preserving the existing prefix value and fixed-buffer safety.

Source: Linters/SAST tools

tests/st/a2a3/host_build_graph/worker_async_fifo/kernels/orchestration/pipelined_vector_orch.cpp (1)

18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated orchestration kernel differing only in kChainLength.

This file is otherwise identical to tests/st/a2a3/host_build_graph/worker_async_endpoint/kernels/orchestration/long_vector_orch.cpp; consider a single shared source with the chain length injected as a compile definition.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@tests/st/a2a3/host_build_graph/worker_async_fifo/kernels/orchestration/pipelined_vector_orch.cpp`
around lines 18 - 20, Consolidate the duplicated orchestration kernel by reusing
the shared implementation from long_vector_orch.cpp, and remove the duplicate
source-specific logic from the worker_async_fifo path. Inject the differing
kChainLength value of 512 through the build configuration as a compile
definition, while preserving the existing kernel behavior and constants.

tests/st/a2a3/host_build_graph/worker_async_fifo/test_worker_async_fifo.py (1)

161-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Capture the submitter thread's exception for a diagnosable failure.

If st_worker.submit(third_graph) raises inside the daemon thread, the exception is discarded and the test fails at Line 169 with a misleading "did not enter after the first run freed its slot" message.

♻️ Record the failure

-            submitter = threading.Thread(
-                target=lambda: third_result.setdefault("handle", st_worker.submit(third_graph)), daemon=True
-            )
+            def _submit_third():
+                try:
+                    third_result["handle"] = st_worker.submit(third_graph)
+                except BaseException as exc:  # noqa: BLE001
+                    third_result["error"] = exc
+
+            submitter = threading.Thread(target=_submit_third, daemon=True)

and assert third_result.get("error") is None before Line 176.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/st/a2a3/host_build_graph/worker_async_fifo/test_worker_async_fifo.py`
around lines 161 - 169, Update the third-graph submitter thread around
st_worker.submit to catch any exception and store it in third_result["error"]
alongside the handle. Before the existing post-release callback assertion,
assert that third_result.get("error") is None so submission failures are
reported directly while preserving the current admission-capacity checks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes